Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

op-txproxy: external validating proxy for conditional transactions #42

Merged
merged 4 commits into from
Sep 30, 2024
Merged

op-txproxy: external validating proxy for conditional transactions #42

merged 4 commits into from
Sep 30, 2024
+1,205 −0

Conversation

hamdiallam
Copy link
Contributor

@hamdiallam hamdiallam commented Aug 14, 2024 •
edited
Loading

Description
A passthrough proxy service for some endpoint prior to reaching the sequencer. Currently just the sendRawTranscationConditional endpoint. This proxy implements stateless validation as DDoS prevention for this new endpoint

  • Authentication: flashbot like auth. Under the X-Optimism-Signature header
  • Rate Limiting
  • Metrics
  • Enable/Disable Conditional Requests
  • 4337 Entrypoint tx target restriction
  • Basic conditional validation

DevX is responsible for maintenance of this service & any new changes that may be required with usage. Ideally working closely with DevInfra on the proxyd integration & setup

  • The passthrough proxy will rely on the proxyd to broadcast the conditional tx to all replicas as these txs are not p2p gossiped.

Tests
The sequencer endpoint has it's own e2e test in the monorepo and tests in op-geth

The PR includes unit tests for this passthrough endpoint. Testing authentication & entrypoint validation

This endpoint can reject traffic behind a flag. Plan being to enable on testnet and ensure the endpoint & dashboards are working correctly with a live bundler prior to enabling on mainnet

Additional Context
Requires the PR for the sequencer endpoint to land in op-geth.

Metadata
Re-opening of ethereum-optimism/optimism#11223. This repo is better suited for this service as supporting infrastructure

@hamdiallam hamdiallam requested a review from a team as a code owner August 14, 2024 15:41
hamdiallam
hamdiallam commented Aug 14, 2024
View reviewed changes
op-txproxy/go.mod Outdated Show resolved Hide resolved
jelias2
jelias2 reviewed Aug 21, 2024
View reviewed changes
Copy link
Contributor

@jelias2 jelias2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hamdiallam would you be able to include a README describing op and how it would fit into our infrastructure?

I'd just want to make sure this fits within the context of the infra repo as infra team is mostly likely the people who will may have to maintain this in the long run

@hamdiallam
Copy link
Contributor Author

Hi @hamdiallam would you be able to include a README describing op and how it would fit into our infrastructure?

I'd just want to make sure this fits within the context of the infra repo as infra team is mostly likely the people who will may have to maintain this in the long run

yes I definitely get a README added here

@hamdiallam
Copy link
Contributor Author

@jelias2 added a readme

@axelKingsley axelKingsley mentioned this pull request Sep 10, 2024
Merged
anacrolix
anacrolix suggested changes Sep 13, 2024
View reviewed changes
Copy link

@anacrolix anacrolix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there might be a leak.

It's nice to put the auth into the context. Why can't the "next" Handlers just tee the body and do this themselves with a helper?

op-txproxy/auth_handler.go Outdated Show resolved Hide resolved
op-txproxy/auth_handler.go Outdated Show resolved Hide resolved
op-txproxy/auth_handler.go Outdated Show resolved Hide resolved
tynes
tynes reviewed Sep 13, 2024
View reviewed changes
op-txproxy/README.md Outdated Show resolved Hide resolved
tynes
tynes reviewed Sep 13, 2024
View reviewed changes
op-txproxy/README.md Outdated Show resolved Hide resolved
anacrolix
anacrolix suggested changes Sep 16, 2024
View reviewed changes
op-txproxy/auth_handler.go Outdated Show resolved Hide resolved
@hamdiallam
Copy link
Contributor Author

I think there might be a leak.

It's nice to put the auth into the context. Why can't the "next" Handlers just tee the body and do this themselves with a helper?

Good Q! So because this is using geth lib to construct server, it would require some changes to op-geth in order to allow a json-rpc methods to inspect the request body. See ethereum-optimism/op-geth/pull/352. There were some valid concerns around the required changes.

It seemed simpler to instead create an http middleware as it remains compatible with using geth & actually made this PR a lot more unit-testable

Curious what you think about this

@anacrolix
Copy link

Yeah it made sense after I read a bit further. Cheers

@hamdiallam
Copy link
Contributor Author

@tynes @anacrolix soft bump on the latest changes

anacrolix
anacrolix approved these changes Sep 25, 2024
View reviewed changes
op-txproxy/auth_handler.go Outdated Show resolved Hide resolved
op-txproxy/auth_handler.go Outdated Show resolved Hide resolved
jelias2
jelias2 reviewed Sep 27, 2024
View reviewed changes
op-txproxy/go.mod Outdated Show resolved Hide resolved
jelias2
jelias2 reviewed Sep 27, 2024
View reviewed changes
Copy link
Contributor

@jelias2 jelias2 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, two small items

I would recommend adding the op-txproxy to the tag-service file here so the repo can publish releases.

Would you also mind adding an appropraite CODEOWNERS entry in .github for this directory

@hamdiallam hamdiallam merged commit dff24e9 into main Sep 30, 2024
22 checks passed
@hamdiallam hamdiallam deleted the op-txproxy branch September 30, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tynes tynes tynes left review comments

@anacrolix anacrolix anacrolix approved these changes

@jelias2 jelias2 jelias2 approved these changes

@protolambda protolambda Awaiting requested review from protolambda

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hamdiallam @anacrolix @tynes @jelias2